[R-package] Add remainder of prediction funtions #5312
Conversation
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
|
Updated. |
There was a problem hiding this comment.
Finally, a feature making it into the R package before the Python package 🎉 . Thanks so much for your help with this!
I'm ready to start actively reviewing this PR and working with you on getting it merged. Overall, I support the approach taken here. I support the introduction of lgb.configure_fast_predict() and really appreciate the detailed documentation and tests.
Please see my next round of review comments. Please also update this to the latest state of the master branch.
I'd like to also run the project's valgrind checks over this code, although right now that CI job is failing on master (#5403 (comment)).
@shiyu1994 or @guolinke once this PR has been updated to latest master, can you please review these changes? Especially in lightgbm_R.cpp and lightgbm_R.h.
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
|
The failing check is not related to this PR: https://dev.azure.com/lightgbm-ci/lightgbm-ci/_build/results?buildId=13270&view=logs&j=582743ec-83fe-58c4-937e-4197b5816801&t=63b016c8-8965-547a-fab6-395af921c96a&l=75 |
|
@guolinke Could you please kindly review this, if you have time? Thanks. |
There was a problem hiding this comment.
Thanks so much for this excellent work! I noticed one typo in docs, but otherwise I am good with these changes and excited to merge them into the project. 🎉
Leaving a "request changes" review just to block merging until we have a chance to run the project's valgrind checks over this PR. And that can't be done until we fix those tests (ongoing work in #5429).
Until then... @jmoralez would you please review this when you have time?
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
|
/gha run r-valgrind Workflow R valgrind tests has been triggered! 🚀 Status: success ✔️. |
|
Sorry I haven't had much time. I'll review this tomorrow |
|
Thanks @jmoralez ! |
|
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
This PR adds the remainder of prediction functions from PR #4982 which were not included in PR #5108 . The idea in #4982 was to split the additions into multiple PRs, but it's getting very complicated to merge branches and to remember what were the changes after the ellapsed amount of time and PRs in between, so this PR just adds the rest of C prediction functions that are not in the R interface together.
It should be pointed out again from issue #4980 that there are some differences between the documentation of some existing C functions added to the R interface here and their actual behavior, and as such the docs will need to be updated once those issues are sorted out.